feat(data-access): add llmBackend field to Organization entity#1588
feat(data-access): add llmBackend field to Organization entity#1588papineni05 wants to merge 4 commits intomainfrom
Conversation
Adds llmBackend attribute to the Organization schema and model with
validation against the allowed values ('azure' | 'bedrock') and a
default of 'azure'. Exposes LLM_BACKEND_AZURE, LLM_BACKEND_BEDROCK,
and LLM_BACKENDS constants for type-safe use by consuming services.
This pairs with the mysticat-data-service migration that adds the
llm_backend column to the organizations table. Together they enable
dynamic per-org LLM backend selection in Mystique, replacing the
hardcoded _OSS_ORG_IDS frozenset.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ST before migration
|
This PR will trigger a minor release when merged. |
solaris007
left a comment
solaris007
left a comment
There was a problem hiding this comment.
Hey @papineni05,
Faithful implementation of the entity layer. Two things worth addressing before merge.
Issues
Important (Should Fix)
packages/spacecat-shared-data-access/test/fixtures/organizations.fixture.js:18 - Only the first fixture record gets llmBackend: 'azure'. Other organizations in the same fixture file are not updated. If any test reads a non-first fixture and expects getLlmBackend() to return 'azure', it may get undefined. Either add llmBackend: 'azure' to all records in the fixture, or rely on a schema-level default (see next finding).
packages/spacecat-shared-data-access/src/models/organization/organization.schema.js:43 - The llmBackend attribute has no default value in the schema. The DB has NOT NULL DEFAULT 'azure', so inserts that omit the column are filled by the DB. But entity consumers calling org.getLlmBackend() on a freshly constructed (unsaved) instance get undefined, not 'azure'. Adding default: 'azure' to the addAttribute config makes the entity layer consistent with the DB layer and removes a class of subtle bugs.
Minor (Nice to Have)
packages/spacecat-shared-data-access/src/models/organization/organization.schema.js:44 - The type: Organization.LLM_BACKENDS form sets type to an array, which ElectroDB treats as enum, AND the validate function double-checks membership. The validate is redundant given the type enum. Either is fine, just unusual to see both.
Recommendations
- Tests cover the getter/setter and constants well. Consider adding a "default behavior on construction" test once the schema default is added.
Assessment
Ready to merge? With fixes.
Reasoning: The schema default and fixture completeness are small but matter for downstream consumers reading the field on uninitialized or non-first-record orgs.
…re records - organization.schema.js: add default: LLM_BACKEND_AZURE so in-memory Organization instances return 'azure' from getLlmBackend() without a DB round-trip — previously getLlmBackend() returned undefined for new records not yet flushed to DB. - organizations.fixture.js: add llmBackend: 'azure' to records at index 1 and 2 so all fixture records are consistent with the schema default. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
solaris007
left a comment
solaris007
left a comment
There was a problem hiding this comment.
Hey @papineni05,
Thanks for the quick turnaround. Both Important findings from the prior review are cleanly addressed in commit baf4cf5e.
Strengths
Previously flagged issues now resolved:
- Schema default added (
organization.schema.js:45):default: Organization.LLM_BACKEND_AZUREmakes the entity layer consistent with the DB-levelNOT NULL DEFAULT 'azure'. Fresh entity instances now return'azure'fromgetLlmBackend()rather thanundefined. - Fixture completeness (
organizations.fixture.js:49, 80): both other organization records now havellmBackend: 'azure'so any test reading non-first fixtures gets a defined value.
Assessment
Ready to merge: Yes
Reasoning: The two Important findings are addressed cleanly with no other behavioral changes. The remaining stylistic Minor from the prior review (type + validate redundancy in the schema) is non-blocking and can be left or cleaned up at the contributor's discretion.
Note: CI checks are currently failing. The Test job errors in seedV2Fixtures with null value in column "organization_id" of relation "sentiment_topics", which is in a different table than this PR touches. The PR description attributes IT-test failures to the dependency on the companion mysticat-data-service PR for the llm_backend column. Worth confirming the failure resolves once that companion PR merges, since the surface error is on sentiment_topics, not on organizations.llmBackend directly.
3 participants
Jira: SITES-43799
Summary
llmBackendattribute to theOrganizationschema ('azure' | 'bedrock', default'azure')LLM_BACKEND_AZURE,LLM_BACKEND_BEDROCK, andLLM_BACKENDSconstants toOrganizationmodelllm_backendcolumn to theorganizationstableContext
Part of the Configurable LLM Backend initiative. Mystique currently routes LLM calls based on a hardcoded
_OSS_ORG_IDSfrozenset. This PR is Step 2 of replacing that with a DB-driven preference: thellmBackendfield is exposed via theOrganizationentity so any service can read or update an org's backend preference through the standard data-access layer.Usage example (consuming service):
Routing once Mystique Step 3+4 ships:
getLlmBackend() === 'azure'→{OPTIMIZER}_LITELLM_KEY→ Azure OpenAIgetLlmBackend() === 'bedrock'→OSS_ORG_LITELLM_KEY→ AWS BedrockChanges
organization.schema.jsllmBackendattribute with enum validationorganization.model.jsLLM_BACKEND_AZURE,LLM_BACKEND_BEDROCK,LLM_BACKENDSconstantsorganization.model.test.jsorganizations.fixture.jsllmBackend: 'azure'to first fixture recordTest plan
npm test -w packages/spacecat-shared-data-access— all tests passingorg.getLlmBackend()returns'azure'for existing orgs (default)org.setLlmBackend('bedrock')thenorg.getLlmBackend()returns'bedrock''gcp') fails schema validationSpec
Configurable LLM Backend — Gap Analysis and Recommendations
🤖 Generated with Claude Code